PR Analysis

• 🎯 Main theme: Addition of new Custom Resource Definitions (CRDs) to the software composition
• 📝 PR summary: This PR introduces new control Custom Resource Definitions (CRDs) in the software composition. The new CRDs include Framework, Control, Rule, Exception, and ControlConfiguration. Each of these CRDs has been defined with respective list types and necessary fields. The changes have been made in the register.go and types.go files under the pkg/apis/softwarecomposition and pkg/apis/softwarecomposition/v1beta1 directories. The PR also updates the k8s.io/apimachinery version in go.mod.
• 📌 Type of PR: Enhancement
• 🧪 Relevant tests added: No
• ⏱️ Estimated effort to review [1-5]: 4, due to the complexity of the changes and the need to understand the context of the new CRDs being introduced.
• 🔒 Security concerns: No security concerns found

PR Feedback

• 💡 General suggestions: The PR introduces new CRDs which are a significant change to the system. It would be beneficial to include tests that verify the correct behavior of these new CRDs. Additionally, it would be helpful to provide more context in the PR description about the purpose and usage of these new CRDs.

• 🤖 Code feedback:

  • relevant file: pkg/apis/softwarecomposition/types.go
    suggestion: Consider adding error handling for the case when the object is not of type Framework in the GetAttrs function. This will prevent potential runtime errors. [important]
    relevant line: "+ apiserver, ok := obj.(*softwarecomposition.Framework)"

  • relevant file: pkg/apis/softwarecomposition/types.go
    suggestion: It would be beneficial to add validation for the new CRDs in the Validate function. This will ensure that only valid objects are processed further. [important]
    relevant line: "+ return validation.AlwaysValid(framework)"

  • relevant file: pkg/apis/softwarecomposition/types.go
    suggestion: Consider adding a comment explaining the purpose and usage of the new CRDs. This will improve code readability and maintainability. [medium]
    relevant line: "+type Framework struct {"

  • relevant file: go.mod
    suggestion: Ensure that the update to k8s.io/apimachinery from v0.26.2 to v0.28.2 does not introduce any breaking changes or compatibility issues. [important]
    relevant line: "+ k8s.io/apimachinery v0.28.2"

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

@vladklokun as discussed, I've been working on this for the past week, right now I'm stuck a bit as I just can't seem to be able to successfully run the codegen script hack/update-codegen.sh.

The script always seems to error out/give out warnings? Not even sure what they are, every time I run it but can't seem to generate the required deepcopy methods, also why they are not present in the PR.

I've looked for solutions online but can't seem to get it working still, any help regarding this would be extremely helpful! Thanks!

The script always seems to error out/give out warnings? Not even sure what they are, every time I run it but can't seem to generate the required deepcopy methods, also why they are not present in the PR.

Yes, the script always generates warnings when running, even after successfully generating deep copy methods, converters and OpenAPI definitions. That’s normal.

You can tell your script ran okay-ish when it generates all the methods you need.

Regarding the missing methods, I know it might sound exotic, but where have you cloned your project? There is a catch with hack/update_codegen.sh that it outputs its definitions in the $HOME/github.com/kubescape/storage/pkg directory, provided you installed the dependencies with the go mod vendor method. To you have generated files in this directory, by chance?

I did check this earlier as well, but that doesn't seem to be the case either, any things I should try and troubleshoot?

I was going to take a fresh look at it later again today :)

@vladklokun finally got the codegen working, could you maybe take a look and let me know if I've missed something before I go ahead and resolve the merge conflicts?

Thanks!

PR Analysis

• 🎯 Main theme: Adding new Custom Resource Definitions (CRDs) and their related functionalities to the software composition schema.
• 📝 PR summary: This PR introduces several new Custom Resource Definitions (CRDs) and their related functionalities: Control, Rule, Framework, and Exception. It includes the necessary conversion functions for the software composition scheme and updates the clientset, listers, and informers to handle these new CRDs.
• 📌 Type of PR: Enhancement
• 🧪 Relevant tests added: No

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

Hi! I finally had a chance to look at the PR. Thank you for your patience.

Type definitions and wiring look good to me.

However, we just merged a PR that includes changes to codegen. Please rebase and try to fix conflicts with the base branch and try to verify that the Custom Resources work. A good start to manually test this would be to create example Custom Resources in the artifacts/{rule,framework,control,...} directory, deploy your APIServer in a cluster using the manifests in the artifacts/example directory and then to create the example Custom Resources you put in the artifacts subdirectories.

Sure!

Will do that and get back to you on this.

Thanks!

@vladklokun have made the required changes! Thanks!

@XDRAGON2002 Can you please commit the go.sum file as well so the test will pass.

@dwertent Completely forgot about that! Thanks! Done!

This is weird, the tests still failed due to the go.mod/sum dependency issue, I already did push the updated ones though.

Edit: Most probably got it, I was using go1.20 whereas some new packages require 1.21, did the change updated everything and pushed, should pass now.

@vladklokun @dwertent I'm pretty sure the tests will fail again, I can't seem to figure out why it is so.

Any help would be much appreciated!